Remove @return(undefined_to_opt) and %undefined_to_opt primitive #7462
Conversation
rescript
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/darwin-arm64
@rescript/win32-x64
commit: |
|
There's some behaviour change after this PR, which can be illustrated through array This pops an array with a single element and prints whether the resulting value is absent, or prints the value inside the option if present https://rescript-lang.org/try?version=v11.1.4&module=esmodule&code=DYUwLgBGIM5gCgewA4QLwQG7oHwCgIIYB3ASzAGMALCAKRgDoBBAJxYEMBPB5FACgDamALoBKCAG8CEAD4QAcogB2IXBADCymIlANgiAOZ8AROwBGMEErDHR0uQGVEAWxB8AHuLQ4NWnSD1DDztCAF88PE0lbV19I2MAHSUAKmSIFDBSZQAeUmsfVNs8aDgkZD5FFTsShH4nVz4AZlE7SL9YoMSUtIyspWzenLywHALkopqyvnq3SpAW4tha8pmmhaA If we store in the array an element of If we move to elements of So the more legit case of storing a single optional value in the array does not behave in the expected way anyway. |
cknitt
force-pushed
the
remove-return-undefined-to-opt
branch
from
2eddf54 to
fbb3a5c
Compare
There was a problem hiding this comment.
One more round on this.
We do want to simplify the compiler, yet avoid to change observable behaviour.
In particular, the conversion from undefined to option should have the expected semantics: x => (x === undefined) ? None : (Some(x))
Will add a small commit on top to address the changes.
| @@ -719,8 +715,6 @@ let result_wrap loc (result_type : External_ffi_types.return_wrapper) result = | |||
| | Return_null_to_opt -> prim ~primitive:Pnull_to_opt ~args:[result] loc | |||
| | Return_null_undefined_to_opt -> | |||
| prim ~primitive:Pnull_undefined_to_opt ~args:[result] loc | |||
| | Return_undefined_to_opt -> | |||
There was a problem hiding this comment.
Removing @return(undefined_to_opt) sounds good, as it does not really have a clear use case.
| @@ -298,8 +298,8 @@ let rec apply ?(ap_transformed_jsx = false) fn args (ap_info : ap_info) : t = | |||
| Lprim | |||
| { | |||
| primitive = | |||
| ( Pundefined_to_opt | Pnull_to_opt | Pnull_undefined_to_opt | |||
There was a problem hiding this comment.
Removing the primitive to convert undefined to option makes sense, as the undefined type is de-emphasized (no Stdlib_undefined module), so it would be weird to special case this conversion in the compiler.
| @@ -31,7 +31,7 @@ type container<'hash, 'eq, 'c> = { | |||
| } | |||
|
|
|||
| module A = Belt_Array | |||
| external toOpt: opt<'a> => option<'a> = "%undefined_to_opt" | |||
| external toOpt: opt<'a> => option<'a> = "%identity" | |||
There was a problem hiding this comment.
This change is fine as it is internal to Belt, and produces identical code.
runtime/Js.res
Outdated
| @@ -190,7 +190,7 @@ type nullable<+'a> = Js_null_undefined.t<'a> = Value('a) | @as(null) Null | @as( | |||
| type null_undefined<+'a> = nullable<'a> | |||
|
|
|||
| external toOption: nullable<'a> => option<'a> = "%nullable_to_opt" | |||
| external undefinedToOption: undefined<'a> => option<'a> = "%undefined_to_opt" | |||
| external undefinedToOption: undefined<'a> => option<'a> = "%identity" | |||
There was a problem hiding this comment.
This is an observable behaviour change, need to use an existing conversion function (now that the primitive does not exist anymore).
| @@ -245,7 +245,6 @@ Js.Array.pop(empty) == None | |||
| ``` | |||
| */ | |||
| @send | |||
| @return(undefined_to_opt) | |||
There was a problem hiding this comment.
These are fine, as a conversion from undefined to optional seems arbitrary here.
runtime/Js_undefined.res
Outdated
| @@ -26,8 +26,8 @@ | |||
|
|
|||
| type t<+'a> = Primitive_js_extern.undefined<'a> | |||
|
|
|||
| external to_opt: t<'a> => option<'a> = "%undefined_to_opt" | |||
| external toOption: t<'a> => option<'a> = "%undefined_to_opt" | |||
| external to_opt: t<'a> => option<'a> = "%identity" | |||
There was a problem hiding this comment.
Behavioural change exposed to the user, need to preserve original semantics.
|
@cristianoc Thanks a lot! Will rebase to get it to build again. |
cknitt
force-pushed
the
remove-return-undefined-to-opt
branch
from
d222fc7 to
c46839d
Compare
produces compiler output like
which doesn't make any sense. This is because it is a leftover from the days when options were compiled differently and
Nonewas not represented asundefinedin the JS output yet.As this feature was never documented, remove it.
Similarly, I think we can replace
%undefined_to_optby%identity. This leads to slight changes in the compiler output. @cristianoc could you have a look to check if everything is indeed fine?